Claude finished @carlos-alm's task —— View job

PR Review Analysis

Reviewing PR #680: fix(test): remove constant-kind exclusion from parity test

Todo List:

• Phase 1: Root cause analysis - understand what problem this PR solves
• Phase 2: Configuration change detection - check for high-risk changes
• Phase 3: Test integrity - verify tests aren't being weakened
• Phase 4: Code quality assessment
• Phase 5: Backlog compliance rating
• Final Assessment: Provide comprehensive review and recommendation

Root Cause Analysis

Problem Being Solved: Removing a test workaround that masked an engine parity bug. The native Rust engine previously extracted function-scoped const declarations as top-level constants, while WASM correctly limited them to program-level scope.

Why This Approach: The underlying bug was fixed in #649/#327 by adding proper find_parent_of_types() scope guards to the native engine. Both engines now use identical function scope type lists, making the test filter unnecessary.

Risk Assessment: LOW RISK - This is removing a workaround after the root cause was fixed, not masking a problem.

Backlog Compliance

• Zero-dep: ✓ — No new dependencies, removes test code only
• Foundation-aligned: ✓ — Improves test coverage and engine parity validation
• Problem-fit: 4 — Directly improves dual-engine reliability and prevents future parity regressions
• Breaking: No — Test-only change, no API or behavior changes
• Tier: 1

Critical Concerns

NONE - This is exactly the right approach after a root cause fix.

Analysis Summary

Phase 2: Configuration Changes ✅ PASS

• No high-risk changes detected
• Only removes test workaround code (11 lines deleted, 2 lines added)
• No build config, linting rules, or timeout changes

Phase 3: Test Integrity ✅ PASS

• Tests are STRENGTHENED, not weakened
• Removes kind != 'constant' filters that were hiding engine differences
• Now validates that both engines produce identical constant extraction
• Perfect example of removing workaround after fixing root cause
• Comments clearly document this was a temporary workaround: "PARITY BUG (fix(native): constant extraction scope bug — local const variables extracted as top-level constants #676): Native engine extracts local const variables... Remove this exclusion once fix(native): constant extraction scope bug — local const variables extracted as top-level constants #676 is resolved."

Phase 4: Code Quality ✅ PASS

• Clean removal of workaround code
• Proper commit message explaining the change
• References the correct issue numbers (fix(native): constant extraction scope bug — local const variables extracted as top-level constants #676, bug: WASM engine extracts 26 fewer nodes than native (engine parity gap) #649)
• No potential bugs or security concerns

Phase 5: Backlog Compliance ✅ PASS

• Zero-dep: ✓ No new runtime dependencies
• Foundation-aligned: ✓ Improves dual-engine parity validation
• Problem-fit: 4/5 - Directly prevents engine parity regressions
• Breaking: No - Test-only change
• Tier 1 classification

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is the correct approach - removing a test workaround after fixing the underlying engine bug. The change strengthens test coverage by validating constant extraction parity between engines. Well-documented, low-risk, and directly improves reliability.

Minor note: Rating 4/5 instead of 5/5 only because this is a cleanup task rather than adding new functionality. The implementation is flawless for what it does.

Greptile Summary

This PR removes the kind != 'constant' workaround from all three parity queries in build-parity.test.ts, restoring full engine-parity coverage for constant-kind nodes, edges, and roles. The workaround was introduced to mask a native engine scope bug (local const inside functions being emitted as top-level constants) that has since been resolved in #649 by aligning FUNCTION_SCOPE_TYPES across both engines.

Key changes:

• nodes query: WHERE kind != 'constant' filter removed — all node kinds are now compared.
• edges query: WHERE n1.kind != 'constant' AND n2.kind != 'constant' filter removed — edges touching constant nodes are now compared.
• roles query: AND kind != 'constant' filter removed — role rows for constants are now compared.
• The explanatory bug comment block is also removed, keeping the file clean.

The change is consistent with the test file's own declared policy ("Do not weaken, skip, or filter this test to work around missing engine parity. Fix the code instead."). No logic, structure, or other test behaviour is affected. The skipped ast_nodes test (tracking separate issue #674) is unchanged and remains correctly deferred.

Confidence Score: 5/5

Safe to merge — removes a narrow workaround filter after the underlying engine bug was fixed, tightening test coverage with no risk of false positives.

The change is minimal, purposeful, and self-consistent: it removes three symmetric filter clauses that masked a now-fixed engine bug. No new logic is introduced, the test structure is unchanged, and the removal is explicitly endorsed by the file's own policy comment. No P0/P1 issues found.

No files require special attention.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant Test as Parity Test
    participant WASM as WASM Engine
    participant Native as Native Engine
    participant DB_W as WASM SQLite DB
    participant DB_N as Native SQLite DB

    Test->>WASM: buildGraph(wasmDir, { engine: 'wasm' })
    WASM-->>DB_W: INSERT nodes (all kinds incl. constants)
    WASM-->>DB_W: INSERT edges
    WASM-->>DB_W: INSERT roles

    Test->>Native: buildGraph(nativeDir, { engine: 'native' })
    Native-->>DB_N: INSERT nodes (all kinds incl. constants)
    Native-->>DB_N: INSERT edges
    Native-->>DB_N: INSERT roles

    Note over Test: readGraph() — no kind filter (post #649 fix)

    Test->>DB_W: SELECT name, kind, file, line FROM nodes ORDER BY ...
    Test->>DB_N: SELECT name, kind, file, line FROM nodes ORDER BY ...
    Test->>Test: expect(nativeGraph.nodes).toEqual(wasmGraph.nodes)

    Test->>DB_W: SELECT ... FROM edges JOIN nodes ... ORDER BY ...
    Test->>DB_N: SELECT ... FROM edges JOIN nodes ... ORDER BY ...
    Test->>Test: expect(nativeGraph.edges).toEqual(wasmGraph.edges)

    Test->>DB_W: SELECT name, role FROM nodes WHERE role IS NOT NULL ORDER BY ...
    Test->>DB_N: SELECT name, role FROM nodes WHERE role IS NOT NULL ORDER BY ...
    Test->>Test: expect(nativeGraph.roles).toEqual(wasmGraph.roles)

_{Reviews (1): Last reviewed commit: "fix(test): remove constant-kind exclusio..." | Re-trigger Greptile}